Skip to content

[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp neighbor commands - #1391

Open
hillol-nexthop wants to merge 7 commits into
facebook:mainfrom
nexthop-ai:bgpneighbor
Open

[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp neighbor commands#1391
hillol-nexthop wants to merge 7 commits into
facebook:mainfrom
nexthop-ai:bgpneighbor

Conversation

@hillol-nexthop

@hillol-nexthop hillol-nexthop commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Stacked on #1345 (config protocol bgp global). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the config protocol bgp neighbor commit here. Merge the lower PRs first.

Stack (bottom → top): #1446#1345#1391 (this PR)#1395#1401#1485#1475#1486#1477#1487#1488#1489#1490#1491

Summary

Adds fboss2-dev config protocol bgp neighbor <ip-address> [<attribute> <value> ...] as a single dispatcher mirroring the global command: the neighbor address is the first positional token, one- or two-token attributes (timers hold-time, add-path send, ...) are matched longest-prefix-first, and handlers mutate the typed bgp::thrift::BgpPeer via ConfigSession::getBgpConfig() / saveBgpConfig(). Also adds fboss2-dev delete protocol bgp neighbor <ip-address>. The 26 old per-attribute peer command classes and the now-dead per-peer folly::dynamic BgpConfigSession API are deleted.

Attribute BgpPeer field
remote-asn / local-asn remote_as_4_byte / local_as_4_byte (4-byte-bounded)
description, peer-tag, peer-group, peer-id, type description, peer_tag, peer_group_name, peer_id, type
ingress-policy / egress-policy ingress_policy_name / egress_policy_name
rr-client, confed-peer, redistribute-peer, enhanced-route-refresh, next-hop-self matching bools
connect-mode <PASSIVE|ACTIVE> is_passive
afi disable-ipv4-afi / disable-ipv6-afi / ipv4-over-ipv6-nh disable_ipv4_afi / disable_ipv6_afi / v4_over_v6_nexthop
add-path send|receive <bool> merged into the AddPath enum (RECEIVE/SEND/BOTH)
bind-addr address, next-hop4, next-hop6 local_addr, next_hop4, next_hop6 (IP/family-validated)
graceful-restart restart-time / stateful-ha bgp_peer_timers.graceful_restart_seconds / enable_stateful_ha
timers hold-time / keepalive / out-delay / withdraw-unprog-delay bgp_peer_timers.*
max-route pre-filter / post-filter / {pre,post}-warning-threshold / {pre,post}-warning-only pre_filter / post_filter RouteLimit fields
link-bandwidth, advertise-lbw, receive-lbw link_bandwidth_bps, advertise_link_bandwidth, receive_link_bandwidth (enum-membership-validated)

Bare neighbor <ip> creates the peer; attribute sets find-or-create it, keyed by the folly::IPAddress-normalized peer_addr (prefix notation for passive sessions accepted). delete protocol bgp neighbor <ip> removes the matching peer via the same shared normalize/find helpers (BgpPeerAddr.h); deleting an unknown neighbor errors without persisting anything.

Behavior notes

  • New peers are seeded with parseable any-addresses (local_addr/next_hop4/next_hop6 = 0.0.0.0/::): bgpd constructs folly::IPAddress from these non-optional fields at config load and aborts on the empty strings a fresh peer would otherwise serialize (IPAddressFormatException in Config::createCommonPeerGroupConfig, reproduced on a test device — the daemon crash-looped into systemd's start limit). The pre-existing sparse-JSON path had the same latent bug.
  • Rejected instead of persisting dead config (precedent: cluster-id in the global PR): connect-mode BOTH (thrift only models is_passive), peer-port / bind-addr port (no per-peer port field; the daemon uses the global listen_port), afi ipv4/ipv6-labeled-unicast (no thrift field). A rejected value never lands on disk.
  • CLI11 token stealing fixed: parent-chain subcommand fallthrough reclassified attribute tokens matching sibling command names (neighbor <ip> peer-group X misparsed with X as the neighbor address). positionals_at_end() on the neighbor command makes everything after the first positional parse as positionals.
  • bgp_json_to_cli.py peer generators emit the new neighbor grammar (shlex-escaped, including the neighbor address; the shell-injection test is updated accordingly); explicit zero timers/limits are no longer dropped and symbolic add_path enum names are handled.
  • Integration tests verify the committed neighbor config through the daemon's TBgpService::getRunningConfig RPC (with a brief connection retry — systemd reports the unit active before the thrift server binds its port), proving bgpd parsed and adopted the promoted config rather than only checking the file the CLI wrote.

Test Plan

Unitbazel test //fboss/cli/...: CmdConfigBgpNeighborTestFixture (14 tests: arg validation/normalization, longest-prefix match, add-path merge matrix, connect-mode, parity attributes, ASN/IP/enum validation, reject paths) and CmdDeleteBgpNeighborTestFixture (5 tests). pytest fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py (67 pass, including the shell-injection test against the new grammar).

Integration on device (real bgpd_cpp daemon):

[  PASSED  ] 11 tests.
  ConfigBgpNeighborTest.SetRemoteAsnAndCommit          (commit; verified via file + getRunningConfig RPC)
  ConfigBgpNeighborTest.SetTimersHoldTimeAndCommit     (commit; nested timers via RPC)
  ConfigBgpNeighborTest.BareCreateStagesPeer
  ConfigBgpNeighborTest.AttributesAccumulateOnOnePeer  (7 attrs incl. the peer-group token-steal regression)
  ConfigBgpNeighborTest.AddPathMergesDirections
  ConfigBgpNeighborTest.InvalidBoolValueRejected
  ConfigBgpNeighborTest.UnsupportedAttributeRejected
  ConfigBgpNeighborTest.ConnectModeBothRejected
  ConfigBgpNeighborTest.DeleteStagedNeighbor
  ConfigBgpNeighborTest.DeleteUnknownNeighborRejected
  ConfigBgpNeighborTest.DeleteNeighborAndCommit        (commit -> delete -> commit; absence verified via RPC)

Sample usage:

$ fboss2-dev config protocol bgp neighbor 2001:db8::99 remote-asn 65510
Successfully set remote-asn to: 65510 for neighbor 2001:db8::99
$ fboss2-dev config protocol bgp neighbor 2001:db8::99 timers hold-time 90
Successfully set timers hold-time to: 90 seconds for neighbor 2001:db8::99
$ fboss2-dev config protocol bgp neighbor 2001:db8::99 connect-mode BOTH
Error: connect-mode BOTH has no representation in bgp_config.thrift (BgpPeer only models is_passive); use PASSIVE or ACTIVE
$ fboss2-dev config session commit
Config session committed successfully as d99c4cd and bgpd (restart) restarted.
$ fboss2-dev delete protocol bgp neighbor 2001:db8::99
Successfully deleted BGP neighbor 2001:db8::99

@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

@meta-codesync

meta-codesync Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D114376255. (Because this pull request was imported automatically, there will not be any future comments.)

@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

Follow-up cleanups to the BGP-aware config session infra (facebook#1344):

- Collapse the BGP_RESTART action level into AGENT_WARMBOOT (bgpd has no hitless
  reload; its restart already runs the agent-warmboot code path).
- Introduce a single ConfigDomain descriptor + configDomains() and shared
  per-domain helpers so commit(), rollback() and `config session diff` handle
  the agent and BGP domains uniformly (private DiffDomain removed).
- Make the agent skip-when-unchanged like BGP: a commit whose staged config
  equals what is already promoted is a true no-op (no git revision, no symlink
  churn, no reloadConfig()/bgpd restart). Change detection is semantic (compare
  the deserialized thrift structs), so formatting-only diffs don't count.
- Consolidate `config session clear` onto a static stagedSessionFilePaths()
  and reuse ConfigSession::readStagedContent() in diff.
- Make ConfigSession::saveConfig(service, level) generic over the service and
  reduce saveBgpConfig() to a thin wrapper.
- Keep the heavy generated thrift headers out of ConfigSession.h: use the
  *_types_fwd.h forward-declaration headers, hold agentConfig_/bgpConfig_ by
  std::unique_ptr, and drop the configLoaded_/bgpConfigLoaded_ bools
  (null == not loaded).
- clang-tidy: use auto for the SimpleJSONSerializer template-cast results.

Built fboss2-dev + the config unit tests; config-session/commit/diff/BGP/clear
tests pass. Verified the agent no-op behaviour live on test switches.
Removed comments about the destructor definition in ConfigSession.h.
…thInterface call

main renamed findFirstEthInterface() to getRandomInterfacePortName()
(virtual-management-port fix); convert the branch-added no-op-commit test
to the new helper. Drop the three includes ConfigSession.cpp no longer
uses directly (misc-include-cleaner runs as errors in CI).
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

Add `fboss2-dev config protocol bgp global <attr> <value>` on top of the
BGP-aware ConfigSession (base PR). Edits the typed bgp::thrift::BgpConfig via
ConfigSession::getBgpConfig()/saveBgpConfig() -- the whole-config,
scope-agnostic typed API (no global/peer/peer-group special-casing in
ConfigSession).

- Collapse the 10 per-attribute global command classes into one dispatcher;
  reject cluster-id (no BgpConfig field) instead of writing dead config; bound
  switch-limit / max_golden_vips so out-of-range values aren't truncated.
- Integration tests: ConfigBgpGlobalTest (each attr set+commit, verified in the
  promoted /etc/coop/bgpcpp/bgpcpp.conf) and ConfigBgpSessionTest
  (clear/diff/commit/rollback + a no-op-restart regression), sharing
  ConfigBgpTestBase.

Test Plan:
- bazel test //fboss/cli/fboss2/test/config:cmd_config_test
- bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test
- fboss2_integration_test on a DUT with bgp_pp active: ConfigBgpSessionTest
  6/6 pass (clear, diff, commit-restarts-bgp_pp, rollback-restores-config, and
  the unchanged-config does-not-restart case); agent-session regression
  (ConfigInterfaceMtuTest) passes.
- Extract the value parsers (parseBool/parseInt/parseNonNegInt32, handler
  Result) into a shared BgpCliValueParsers.h so sibling BGP dispatchers can
  reuse them, and add a bounded parseAsn4Byte: local-asn/confed-asn accepted
  any uint64 and silently persisted out-of-range ASNs (>= 2^32 wrap the i64
  field negative).
- positionals_at_end() on the global command: CLI11's parent-chain subcommand
  fallthrough steals value tokens that match a sibling command name (e.g. a
  policy named "peer-group") and misparses the command.
- Integration test base: probe the bgpd unit, and pass
  -c safe.directory=/etc/coop on the raw git invocations (gitHead /
  bgpTrackedAtRevision), mirroring the CLI's Git class -- /etc/coop is owned
  by another user (e.g. coop) on provisioned devices, which git otherwise
  rejects as dubious ownership and the helpers silently return empty results.
The commit-path global-attribute tests asserted on the promoted
/etc/coop/bgpcpp/bgpcpp.conf, which only proves what the CLI wrote to disk.
Route ConfigBgpTestBase::setAndCommit() through a new
readRunningBgpConfigViaRpc() -- TBgpService::getRunningConfig against the local
bgpd -- so every positive ConfigBgpGlobalTest asserts its attribute in the
daemon's own view of its config, proving bgpd parsed and adopted the promoted
file after the commit-triggered restart.

The helper retries briefly on connection errors: systemd reports bgpd active as
soon as the process starts, but its thrift server binds the port a few seconds
later, so an RPC issued right after a restart races that window.

Test Plan:
- bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test
- fboss2_integration_test on a DUT with bgpd active: ConfigBgpGlobalTest 7/7
  pass (each attribute set+commit verified in bgpd's getRunningConfig view,
  plus the invalid-bool and negative-graceful-restart-time reject paths).
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

Adds `config protocol bgp neighbor <ip-address> [<attribute> <value> ...]`
and `delete protocol bgp neighbor <ip-address>` — the first per-peer BGP
config family, beneath the `protocol bgp` grouping node.

- neighbor dispatcher keyed by peer address, writing bgp_config.BgpPeer
  through the typed ConfigSession. Attributes cover the peer identity
  (remote/local ASN, description, peer-tag, peer-group), policy bindings
  (ingress/egress policy), session tunables (timers, route limits,
  next-hop, add-path, RR client, and the rest of the documented set), and
  the sheet-documented attributes bgpd does not support, which are
  rejected with an explanatory message rather than staged.
- delete removes the peer by address.
- ConfigBgpTestBase restores the committed BGP config after every test, so
  a failed run cannot leave a device carrying test config; commit-path
  integration tests verify each value through bgpd's own getRunningConfig
  RPC rather than only the staged file.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant